Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

destroy session first on regenerate session when session is active #7

Merged

Conversation

samsonasik
Copy link
Member

Q A
Bugfix yes
BC Break no
New Feature no
RFC no

Description

re-create PR for #1 . There is a use case when session already started before, and already set some value, eg: on csrf session data on a login form. When authenticate, it call the session regenerate, which session id changed, but left the old session not persisted (unset not applied as session id changed), so old session with the value remain in the disk.

To avoid it, I think we can apply session_destroy() before session_write_close() whenever session is active.

@michalbundyra michalbundyra added the Bug Something isn't working label Jan 26, 2020
@michalbundyra
Copy link
Member

@samsonasik I've made some improvements to your test and all seems to be working fine - on travis and on local. Please have a look.

Tests in that file run in separate process so there is no need for clear the session at the end of the test.

@michalbundyra michalbundyra added this to the 1.7.1 milestone Jan 26, 2020
@samsonasik
Copy link
Member Author

@michalbundyra thank you, that's ok 👍

@samsonasik
Copy link
Member Author

@michalbundyra any chance for merging ? thank you.

Copy link
Member

@Xerkus Xerkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned with the change of initial intended behavior to exact opposite.

Why was it done in a way explicitly preserving old session with data? session_regenerate_id(true) could have been used otherwise to achieve what this PR wants to introduce.

Could it possibly be that race condition actually is an issue that cause new session id to be overwritten by one of the parallel requests? As mentioned in docs, say, strict mode regenerating id on empty/missing old session and sending cookie superseding actual session?

@samsonasik
Copy link
Member Author

@Xerkus I've removed session_write_close() function call

@Xerkus Xerkus changed the base branch from master to develop February 4, 2020 09:49
@Xerkus
Copy link
Member

Xerkus commented Feb 4, 2020

This is not a bugfix, but rather improvement that changes behavior. Should target next minor.

@Xerkus
Copy link
Member

Xerkus commented Feb 6, 2020

So far these are the only effect of this change that I can think of:

  1. Refreshed session id is lost on bad connection (like mobile network). Session data is effectively lost as previous session is deleted.
    Example:

    • User comes as a guest
    • Multiple items are placed into the cart
    • On checkout user logs in, session id is regenerated
    • Request fails to load, new session id is lost
    • Cart reference in session is lost. No recovery available.

    Generally we see this as an acceptable very low probability security tradeoff.

  2. With session.use_strict_mode enabled, deleting session might have client side parallel request race condition leading to scenario 1. Strict mode is off by default.
    Example:

    • Browser launches request that will regenerate session id.
    • Browsers launches additional parallel requests with original session id. Those requests are waiting on blocking session_start()
    • Browser receives response with regenerated session id
    • Waiting parallel request proceed once session lock is released by first request. Since session was deleted strict mode kicks in and regenerates session id.
    • With SAPI emitter session cookie is automatically sent (This should probably be investigated in laminas/laminas-httphandlerrunner - reset php queued headers before emitting PSR response)
    • Browser receives parallel response and overwrites proper regenerated session id
    • We got scenario 1.

    I did not try to recreate this scenario.
    If it is an actual issue when strict mode is enabled we can replace session data instead of deleting with say ['__deleted__' => true] . Needs further investigation.

@weierophinney
Copy link
Contributor

Why was it done in a way explicitly preserving old session with data? session_regenerate_id(true) could have been used otherwise to achieve what this PR wants to introduce.

We chose not to use session_regenerate_id() because we needed to be able to call session_start() with custom options (use_cookies, use_only_cookies, and cache_limiter). Unfortunately, when you call session_regenerate_id, PHP sets the set-cookie header itself, preventing us from setting the options; this meant having custom logic when regenerating the cookie.

As such, the approach here by @samsonasik is the correct one.

Copy link
Contributor

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@samsonasik
Copy link
Member Author

@michalbundyra merge-able ?

@michalbundyra
Copy link
Member

@samsonasik Yes, I'll merge it soon, it's on my list, but first I am working on laminas-form.

@samsonasik samsonasik force-pushed the destroy-first-on-regenerate-session branch from fc37047 to 198f5eb Compare March 18, 2020 13:10
@michalbundyra michalbundyra force-pushed the destroy-first-on-regenerate-session branch from 198f5eb to 8ec093e Compare March 19, 2020 18:52
@michalbundyra michalbundyra changed the base branch from develop to master March 19, 2020 18:52
@michalbundyra
Copy link
Member

Thanks, @samsonasik!

michalbundyra added a commit that referenced this pull request Mar 19, 2020
michalbundyra added a commit that referenced this pull request Mar 19, 2020
@michalbundyra michalbundyra merged commit 8a666fd into mezzio:master Mar 19, 2020
@samsonasik samsonasik deleted the destroy-first-on-regenerate-session branch March 19, 2020 20:06
@michalbundyra michalbundyra linked an issue Mar 19, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

destroy session on regenerate session when session is active
4 participants